-
Notifications
You must be signed in to change notification settings - Fork 999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transform update_types in IgnoreCondition #3550
Conversation
030b2c6
to
1e2ae22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM: I'd assumed these exact strings only mattered when parsed directly from the config file.
If we're going to pass the verbatim strings through the external parser+storage and back into here, this works. I'd assumed it would be converted at some point, and intended to use the symbol.
What I really wanted is an enum in the JVM or an iota
constant in Golang. Symbols where my guess at how Ruby might follow the same pattern...
With similar biases, I'd dig if these magic strings were defined and referenced as constants instead of duplicated across the codebase. The pattern in 7375421#diff-67a26681133898ab39f6dac25dad58b8bfa751fd7245ec723c196f1184f93101R9 seemed pretty neat!
Moving the update_types transform from the config file parser to the ignore condition making it easier to initialize IgnoreCondition without fetching a config file.
1e2ae22
to
348d246
Compare
It would be real nice to iterate towards moving all config file validation and parsing to core and using that code from api, this would make it easier to consolidate on transforms in one place. We could then fail jobs if the config is currently invalid which would increase visibility of these issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a nit about the transform.
end | ||
|
||
private | ||
|
||
def transformed_update_types | ||
update_types.map(&:downcase).map(&:strip).compact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_types.map(&:downcase).map(&:strip).compact | |
update_types.compact.map(&:downcase).map(&:strip) |
Nit: this compact
does nothing, as nil values have already raised?
irb(main):002:0> [nil].map(&:downcase)
(irb):2:in `map': undefined method `downcase' for nil:NilClass (NoMethodError)
I'm not sure if it should be refactored to be a guard for the map()
s (the suggestion), or just removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: should this transform happen in the ConfigFile
/ parsing logic?
We might be able to simplify by assuming the caller of IgnoreCondition
has normalized these values: either through ConfigFile
or the mysterious server-side parsing process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Yeah true, might be better to move to the File
class and rely on the api to have done this already
Moving the update_types transform from the config file parser to the
ignore condition making it easier to initialize IgnoreCondition without
fetching a config file.
Working towards being able to do this: